Adding an option to generate typescript enum instead of union of strings #308
Conversation
There was a problem hiding this comment.
Awesome! Thanks for the contribution @labmorales 🎉 Great work!
I added some code-style comments and would like to discuss the handling of boolean enums.
As i'm not to familiar with the generate.ts I'd like to have another pair of eyes on that
src/codegen/generate.test.ts
Outdated
| import generate, { getOperationName } from "./generate"; | ||
| import { printAst } from "./index"; | ||
| import SwaggerParser from "@apidevtools/swagger-parser"; | ||
| import { OpenAPIV3 } from "openapi-types"; | ||
|
|
||
| import ApiGenerator from "./generate"; | ||
| import { OpenAPIV3 } from "openapi-types"; | ||
| import SwaggerParser from "@apidevtools/swagger-parser"; | ||
| import { printAst } from "./index"; |
There was a problem hiding this comment.
Please remove resorting of imports and empty line 2
demo/enumApi.ts
Outdated
| Sold = "Sold", | ||
| } | ||
| export enum Animal { | ||
| true = "true", |
There was a problem hiding this comment.
This seems odd.
From what I understand this might actually change the type of data send to the server and may cause a mismatch when the server would send a actual boolean in its response (would need to check data-(de)serialization to confirm).
As there's no support of booleans in enums within TS should we maybe continue to use unions for "animal": { "enum": [true], "type": "boolean" }?
Is this intended @labmorales?
There was a problem hiding this comment.
A better question would be how a boolean enum is useful on the OpenAPI spec. Could not find anything about that.
The exported enum seems to work with the boolean as the name of the property. But I agree it does not make much sense to use a boolean as an enum key.
I changed the code so that it would use the previous union type in the case of a boolean enum. But would be nice to know what OpenAPI enum boolean actually should mean.
There was a problem hiding this comment.
I agree that this is probably a unicorn-case that should never happen in real-live. Thanks for taking care of it 🦄 👍
| @@ -880,7 +963,8 @@ export default class ApiGenerator { | |||
| Object.assign(stub, { | |||
| statements: cg.appendNodes( | |||
| stub.statements, | |||
| ...[...this.aliases, ...functions] | |||
| ...[...this.aliases, ...functions], | |||
| ...this.enumAliases | |||
| ), | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Would like to have another pair of eyes on the changes here, I have not worked intensely with the generate part so I'm not the best person to review this. Overall this seems reasonable to me
There was a problem hiding this comment.
Just a bit of context on why I did this way. I had to treat Enums as references in order to allow us to reuse already declared, and create new ones. It needed its own "hashmap" cause otherwise, the names could conflict with the references.
There was a problem hiding this comment.
Thanks for elaborating 👍
I actually meant the whole file ;) Would be cool to have @jonkoops opinion on this
There was a problem hiding this comment.
Hey, @Xiphe any updates on this?
|
Pre-Released this as @jonkoops do you have time to take a look here? When I don't hear anything, I'll merge tomorrow |
|
Let's move forward with this 👍 |
|
released as v4.2.0 |
|
Sorry for the late reply, been caught up in some other stuff. But this is all looking good to me 👍 |
|
No problem. Thanks for the post release LGTM :) |
Adds enum generation option
Using the CLI or the API passing the option
--useEnumTypewill convert the enums to typescript enums instead of using a union of strings.This solves #27.
Why not use components
The components property is only available on the OpenAPI v3. Therefore to make it compatible with the OpenAPI v2 I analyzed the enum values and created the enums based on their title property or in the lack of it the name of the property.
Added some tests and documentation to match this new feature.